-
Couldn't load subscription status.
- Fork 15k
[VPlan] Remove SCEVToExpansion mapping (NFC). #164490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
VPlan::SCEVToExpansion isn't needed any longer, as SCEV expansion de-duplication is handled locally in expandSCEVs.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesVPlan::SCEVToExpansion isn't needed any longer, as SCEV expansion de-duplication is handled locally in expandSCEVs. Full diff: https://github.com/llvm/llvm-project/pull/164490.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 167ba553af599..38f49c1a5f84c 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -4166,11 +4166,6 @@ class VPlan {
/// definitions are VPValues that hold a pointer to their underlying IR.
SmallVector<VPValue *, 16> VPLiveIns;
- /// Mapping from SCEVs to the VPValues representing their expansions.
- /// NOTE: This mapping is temporary and will be removed once all users have
- /// been modeled in VPlan directly.
- DenseMap<const SCEV *, VPValue *> SCEVToExpansion;
-
/// Blocks allocated and owned by the VPlan. They will be deleted once the
/// VPlan is destroyed.
SmallVector<VPBlockBase *> CreatedBlocks;
@@ -4427,15 +4422,6 @@ class VPlan {
LLVM_DUMP_METHOD void dump() const;
#endif
- VPValue *getSCEVExpansion(const SCEV *S) const {
- return SCEVToExpansion.lookup(S);
- }
-
- void addSCEVExpansion(const SCEV *S, VPValue *V) {
- assert(!SCEVToExpansion.contains(S) && "SCEV already expanded");
- SCEVToExpansion[S] = V;
- }
-
/// Clone the current VPlan, update all VPValues of the new VPlan and cloned
/// recipes to refer to the clones, and return it.
VPlan *duplicate();
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
index 32e4b8872b1df..77aee757476e7 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.cpp
@@ -32,8 +32,6 @@ bool vputils::onlyScalarValuesUsed(const VPValue *Def) {
}
VPValue *vputils::getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr) {
- if (auto *Expanded = Plan.getSCEVExpansion(Expr))
- return Expanded;
VPValue *Expanded = nullptr;
if (auto *E = dyn_cast<SCEVConstant>(Expr))
Expanded = Plan.getOrAddLiveIn(E->getValue());
@@ -50,7 +48,6 @@ VPValue *vputils::getOrCreateVPValueForSCEVExpr(VPlan &Plan, const SCEV *Expr) {
Plan.getEntry()->appendRecipe(Expanded->getDefiningRecipe());
}
}
- Plan.addSCEVExpansion(Expr, Expanded);
return Expanded;
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually had this patch initially, but got worried about caching -- on second thought, SCEV internally caches and de-duplicates (maybe add this to the commit message?), so this LGTM, thanks!
VPlan::SCEVToExpansion isn't needed any longer, as SCEV expansion de-duplication is handled locally in expandSCEVs. PR: llvm/llvm-project#164490
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/32159 Here is the relevant piece of the build log for the reference |
VPlan::SCEVToExpansion isn't needed any longer, as SCEV expansion de-duplication is handled locally in expandSCEVs. PR: llvm#164490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-commit review.
| VPValue *Expanded = nullptr; | ||
| if (auto *E = dyn_cast<SCEVConstant>(Expr)) | ||
| Expanded = Plan.getOrAddLiveIn(E->getValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can now early-exit here, and below:
| VPValue *Expanded = nullptr; | |
| if (auto *E = dyn_cast<SCEVConstant>(Expr)) | |
| Expanded = Plan.getOrAddLiveIn(E->getValue()); | |
| if (auto *E = dyn_cast<SCEVConstant>(Expr)) | |
| return Plan.getOrAddLiveIn(E->getValue()); |
| /// NOTE: This mapping is temporary and will be removed once all users have | ||
| /// been modeled in VPlan directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this NOTE been taken care of, i.e., all users are modeled in VPlan directly?
| Expanded = Plan.getOrAddLiveIn(U->getValue()); | ||
| } else { | ||
| Expanded = new VPExpandSCEVRecipe(Expr); | ||
| Plan.getEntry()->appendRecipe(Expanded->getDefiningRecipe()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the previous mapping also took care of caching VPExpandSCEVRecipe's, i.e., if getOrCreateVPValueForSCEVExpr() is called twice for the same EXPR. But such cases should be easy to clean up?
Should ExpandedSCEVs be checked if it already has a Value for Expr, before expanding code for it?
VPlan::SCEVToExpansion isn't needed any longer, as SCEV expansion de-duplication is handled locally in expandSCEVs. PR: llvm#164490
VPlan::SCEVToExpansion isn't needed any longer, as SCEV expansion de-duplication is handled locally in expandSCEVs.